Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test/e2e/framework : move AddOrUpdateTaintOnNode to subpackage node #90190

Merged
merged 1 commit into from Apr 30, 2020
Merged

test/e2e/framework : move AddOrUpdateTaintOnNode to subpackage node #90190

merged 1 commit into from Apr 30, 2020

Conversation

tanjunchen
Copy link
Member

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

after moving this function , i will remove the direct dependency for internal k8s.io/kubernetes

Which issue(s) this PR fixes:

ref:#77095

/cc @oomichi
/cc @alejandrox1

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 16, 2020
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 16, 2020
@tanjunchen
Copy link
Member Author

tanjunchen commented Apr 16, 2020 •
edited

after moving this function , i will remove the direct dependency for internal k8s.io/kubernetes
removing the direct dependency for internal k8s.io/kubernetes
due to #74352
/kind cleanup
/priority backlog

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 16, 2020
oomichi
oomichi reviewed Apr 17, 2020
View reviewed changes
test/e2e/framework/node/resource.go Outdated
@@ -38,6 +38,9 @@ import (
clientretry "k8s.io/client-go/util/retry"
e2elog "k8s.io/kubernetes/test/e2e/framework/log"
"k8s.io/kubernetes/test/e2e/system"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to move the TODO before this line because it also needs to be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch.Thanks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @oomichi
hi,how about this now?

@tanjunchen
Copy link
Member Author

/retest

oomichi
oomichi reviewed Apr 20, 2020
View reviewed changes
Copy link
Member

@oomichi oomichi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 20, 2020
alejandrox1
alejandrox1 reviewed Apr 20, 2020
View reviewed changes
test/e2e/framework/node/resource.go Outdated
func AddOrUpdateTaintOnNode(c clientset.Interface, nodeName string, taint v1.Taint) {
err := controller.AddOrUpdateTaintOnNode(c, nodeName, &taint)

// TODO use wrapper methods in expect.go after removing core e2e dependency on node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO use wrapper methods in expect.go after removing core e2e dependency on node

What do you mean by "core e2e dependency on node"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that means framework core package can't dependency sub-package /test/e2e/framework/node , the core package should not depend on the subpackage as much as possible ,the topic is from #81245 .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move E2E Framework into Staging has higher priority.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after moving this function , i will remove the direct dependency for internal k8s.io/kubernetes
removing the direct dependency for internal k8s.io/kubernetes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that the e2e test framework should not import its own subpackages, and that decoupling the e2e test framework and its subpackages is higher priority than any other cleanup work in the e2e test framework.

What isn't clear from that comment is

  1. why you changed the call to ExpectNoError(),
  2. what "core e2e dependency" actually means?
  3. and what dependency is there on "node" (core e2e dependency on node)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @alejandrox1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update the conflict after we get the answer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excuse the delay. Coming around to, and I apologize for the vagueness. I understand the issue here but my comment was about properly communicating it in the comment

TODO use wrapper methods in expect.go after removing core e2e dependency on node

that comment does not properly communicate what you have been discussing here @tanjunchen .
If we are going to leave TODOs in the code base we should also assume that we will forget about them and someone else will eventually find them and try to resolve them.
The comment you have here does fully communicate the work to be done.

A better comment could be something like

TODO use wrapper methods in expect.go after removing the dependency on this 
package from the core e2e framework.

My comment here is just for us to standardize on the language we used to talk about things.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get it,Thanks for your comment,i will update it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oomichi @alejandrox1
done. pls review this again! Thanks

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2020
@tanjunchen
Copy link
Member Author

/retest

alejandrox1
alejandrox1 reviewed Apr 29, 2020
View reviewed changes
Copy link
Contributor

@alejandrox1 alejandrox1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/lgtm

Thank you @tanjunchen !

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2020
@alejandrox1
Copy link
Contributor

/retest

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alejandrox1, tanjunchen

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2020
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@tanjunchen
Copy link
Member Author

/retest

1 similar comment
@tanjunchen
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit 66e3c66 into kubernetes:master Apr 30, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Apr 30, 2020
@tanjunchen tanjunchen deleted the move-20200416 branch April 30, 2020 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oomichi oomichi oomichi left review comments

@alejandrox1 alejandrox1 alejandrox1 left review comments

Assignees

@oomichi oomichi

@alejandrox1 alejandrox1

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tanjunchen @alejandrox1 @k8s-ci-robot @fejta-bot @oomichi